Is mask a typo? If not, I am confused by this verb.

It was not, but I see that it's not accurate… so changed to restrictEntityLookup.

Why have this argument an an if-else? Looks like you can just have two simple methods

True, can even be one simpler method… changed to just use phpunit mocks all the time and not sometimes rely on RestrictedEntityLookup.

In the fundraising codebase we use the convention to name such methods in tests that construct new stuff newStuff and not getStuff. I think that is clearer, since then you know you get a new instance every time and can happily cause changes in its state without other stuff blowing up

I'm also a fan of the new… naming convention.

Good idea, changed.

typo: visited

Done :)

Does anything else depend on this specifically returning an InMemoryEntityLookup and not any EntityLookup?

No, changed the return type hint.

I'm also a fan of the new… naming convention.

Oh, this is confusing. A "maximum depth" is not the same as a "maximum number of entities to search through". This search spans a tree. If the tree is very flat, the search depth might be something like 5, while a few thousand entities have been traversed already. Please count the entities, and not the depth. We run into issues with this in the QualityConstrains codebase already. @lucaswerkmeister might be able to provide an example.

Further down in the constructor I see both a depth and some maximum number. It's hard to tell what they do. I think a depth is not needed.

If you think having both is helpful, I would not make the depth throw an exception. An exception is only thrown if the maximum number of entities is reached. My reasoning for this idea is: Think of a tree with two branches, the first is extremely deep, while the second is only a few entities deep. Both together exceed the "maximum number of entities" setting. If the depth-check throws an exception, nothing would be found. But if the depth-check only makes the code stop traversing a particular branch, the second, smaller branch would be found.

I hope this makes sense.

I hope I correctly understood your comment, if not, please correct me!

Please note that this is not doing a depth-first search, but a breadth-first search, thus there's no need to make "the code stop traversing a particular branch". In this context I think it might be useful for a caller to say that they only want us to look for paths with a certain maximum length.
The entity visit limitation is just an added safety, which I don't imagine firing very often, but only in specific cases where one or more visited entities references many other entities.

I agree that the documentation for this might be improvable. I'll see what I can do there.

I'm afraid I still don't know what this integer represents. The documentation uses the words "depth" (as in how deep a tree is) and "number" (as in the number of visited leaves) in the same sentence. For me these are two very different concepts. Which one does this integer represent?

My understanding is that this represents the maximum depth of the tree. You can also think of the depth as the number of entities above the current leaf (i. e., intermediate entities) at any point. Does that make sense?

You can also think of the depth as the number of entities above the current leaf […]

"Above the current leave" is an other tree. What "number of entities" from this tree is meant? A horizontal or a vertical slice?

Maximum number of intermediate entities to search through, this means given we have a path Q1 -> … -> Q42, this is the number of entities represented by the ….

So "maxDepth" is indeed the maximum depth of that particular tree. :-) Thanks!

Couldn't you keep this cache for the lifetime of the object?

No, because we want to maybe visit the same entities again, if another (or even the same) reference-relationship is queried.
We may have a caching decorator for this in the future, but that's not this classes business.

I miss a safety check here that starts screaming when this is called with an already visited entity. As far as I can see this is not required, as the caller makes sure this can't happen. But this is a little hidden in an array_diff, and I would feel better with an additional isset check. It's cheap.

Added a LogicException here.

Changed this to actually trigger a warning now… throwing an exception is maybe a bit harsh.

trigger_error causes a global side effect. Without knowing the context here I can't say what is appropriate, though normally you either throw an exception and let the caller deal with it, which might be ignoring the error, escalating it, logging it, etc. Or, in cases where the execution in the local context should not be aborted, you log something or otherwise output info via some collaborator (which could also be an event thing or whatever).